Preserve pull request context, full-scan SCM branch pipelines, and gate ignore commands - #302
Preserve pull request context, full-scan SCM branch pipelines, and gate ignore commands#302lelia wants to merge 27 commits into
Conversation
e3e708b to
695703b
Compare
695703b to
3cd0355
Compare
3cd0355 to
0e03a6f
Compare
lelia
changed the title
There was a problem hiding this comment.
[agent] Reviewed via two independent passes (security-focused and quality-focused) over the diff against main, plus manual verification of the two most significant findings against the actual source.
Requesting changes on one high-severity issue and one adjacent bug this PR doesn't touch but sits right next to.
Ignore-comment authorization. Comment.author_association is defined on the Comment dataclass but never read anywhere in the codebase -- there's no check that the person posting @SocketSecurity ignore <pkg>@<version> actually has write access to the repo. That gap predates this PR, but this PR is what makes it exploitable in practice: bare (non-ecosystem-qualified) ignore commands were actually broken before (the old parser threw on any scoped package name and the error was swallowed), and the new scope-disambiguation logic here makes that path work again. See the inline comment on is_ignore in scm_comments.py.
Adjacent crash bug, not touched by this PR. process_original_security_comment (the legacy comment-table format) still does pkg_name, pkg_version = details.split("@") on a scoped package name, which unpacks into 3 parts and raises an uncaught ValueError -- the exact bug class this PR fixes in process_updated_security_comment via rsplit("@", 1), just left unfixed in its sibling function. socketsecurity/core/scm_comments.py:155. Worth fixing in the same pass since it's the same bug, in the same file, one function over -- couldn't attach this as an inline comment since the line isn't part of this diff.
Also flagging, not blocking, both left as inline comments: _github_url/_gitlab_url in pull_request.py build the dashboard-link host from GITHUB_SERVER_URL/CI_SERVER_URL without the scheme/netloc validation _repository_url already applies to the sibling env vars in the same file.
One tiny doc fix, not on a changed line so no inline comment for it: add_purl_capabilities's docstring in socketsecurity/core/__init__.py:2346 still says this only touches new_packages, but the loop now also processes updated_packages.
Everything else checked out: the diffType classification, the create_scm_scan full-scan/blocking refactor, the PR-context resolution precedence, and the argparse-suppress plumbing were all traced by hand and are well covered by the new tests.
| target_names = {name} | ||
| if not pkg_type and "/" in name and not name.startswith("@"): | ||
| target_names.add(name.split("/", 1)[1]) | ||
| return bool(package_names & target_names) and (pkg_version == version or version == "*") |
There was a problem hiding this comment.
This function has no way to know whether the commenter is authorized to ignore an alert -- Comment.author_association exists on the dataclass but is never checked anywhere in the call path. This PR is what makes the bare-name fallback here actually functional (it used to throw on any @-containing name), so it's the right spot to add the check rather than deferring it.
Also: package_names always includes the bare pkg_name regardless of pkg_type, so a bare ignore command matches across every ecosystem with that name+version, not just the one that was actually alerted.
There was a problem hiding this comment.
Fixed in 977a8fb — good catch, and you're right that this was the spot to do it rather than defer.
The gate went into check_for_socket_comments rather than is_ignore, since that's the one place every consumer passes through: remove_alerts, process_security_comment, and both ignore-telemetry loops in socketcli.py all read the already-bucketed dict. A rejected command is therefore also absent from the telemetry, which should record what was acted on.
GitHub is free and definitive — author_association is already in the comment payload, so OWNER/MEMBER/COLLABORATOR only, and a missing value is treated as unauthorized rather than trusted.
GitLab needed more thought. Notes carry no permission field, and my first attempt (GET /projects/:id/members/all/:user_id) turned out to be unsafe here: CliClient.request funnels every HTTP error through raise_for_status() into APIFailure without a status code, so a 404 meaning "not a member" — exactly the outsider case this guards — is indistinguishable from a token that can't read the endpoint, and would have to fail open. Switched to members/all, which answers non-membership with a 200 and an absent id, so the answer is definitive whenever the list is readable. Developer (30) or above, fetched once per run and only when an ignore command is actually present.
One deliberate asymmetry worth flagging: when the GitLab list genuinely can't be read — a CI_JOB_TOKEN generally can't, and that's what the shipped workflows/gitlab-ci.yml template uses — the command is honored and a warning names the author. Failing closed there would break every templated GitLab pipeline that relies on ignore commands. Documented under "Who can ignore an alert" in cli-reference.md with the token needed to get enforcement. Happy to flip it to fail-closed if you'd rather take the breakage.
On the second half — bare names matching across ecosystems — that one I left as-is, deliberately. Pre-2.9.0 the generated command was ignore {pkg_name}@{version} with no ecosystem at all, so a bare command sitting in an existing comment genuinely carries no ecosystem information; name+version is the only thing available to match on. Narrowing it would break the round-trip with older comments that this PR set out to fix. The version still has to match exactly (or be *), and with the authorization check in place the blast radius is bounded to people who could have ignored the alert anyway. I did fix the adjacent real over-match in d49facb, where a leading npm scope was being stripped as though it were an ecosystem, so ignore @types/node@* no longer also matches a package named node.
22 new tests in tests/unit/test_ignore_authorization.py, including ignore-all from an outsider (the more powerful command, and one that did work before this PR).
| repository = env.get("GITHUB_REPOSITORY") or remote_path or repo | ||
| if not repository or "/" not in repository: | ||
| return None | ||
| server = env.get("GITHUB_SERVER_URL") or (f"https://{remote_host}" if remote_host else "") |
There was a problem hiding this comment.
server is taken directly from GITHUB_SERVER_URL with no scheme/netloc validation, unlike _repository_url just above which checks parsed.scheme in ("http", "https") and parsed.netloc for the sibling env vars. Not reachable through standard GitHub Actions (runner-set), but this value becomes external_href sent to the API, so routing it through the same validator would be cheap defense-in-depth.
There was a problem hiding this comment.
Fixed in f16525f. Pulled the scheme/netloc check out of _repository_url into a shared _http_url, and routed GITHUB_SERVER_URL through it alongside the https://{remote_host} fallback derived from BUILDKITE_REPO.
Agreed it's defense in depth rather than a live hole — the runner sets this — but as you say it ends up as external_href on the diff scan, so it's cheap to validate at the boundary.
An unusable value falls back to https://github.com rather than dropping the link, since GitHub has a well-known public host. Covered by a parametrized test over javascript:, a bare string, ftp://, a scheme with no host, and empty, plus one asserting self-hosted GHE URLs still work.
| if not project_url: | ||
| remote_host, remote_path = parse_git_remote(env.get("BUILDKITE_REPO")) | ||
| project_path = env.get("CI_PROJECT_PATH") or remote_path or repo | ||
| server = env.get("CI_SERVER_URL") or (f"https://{remote_host}" if remote_host else "") |
There was a problem hiding this comment.
Same gap as GITHUB_SERVER_URL above -- CI_SERVER_URL is used raw here too.
There was a problem hiding this comment.
Same fix in f16525f — CI_SERVER_URL goes through the same _http_url validator.
One difference from the GitHub side: GitLab has no public default host to fall back to, so an unusable CI_SERVER_URL drops the merge request link entirely rather than guessing gitlab.com. The scan still carries its MR number, it just doesn't get a Dashboard link. Tested both ways, including that a self-hosted http://gitlab.internal with a nested subgroup path still composes correctly.
2.8.0 and 2.8.1 shipped from main while this branch was open, so the original 2.8.0 bump here is dead. This branch changes the behavior of existing flags rather than only fixing them -- --pr-number gains auto-detection, --scm github|gitlab implies --integration, and SCM branch pipelines switch from diff scans to full scans and stop returning a blocking exit code -- so it takes the minor bump per the repo's semver standard, not a patch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The GitHub comment adapter and pull request link construction each parsed BUILDKITE_REPO independently. Consolidate on socketsecurity.core.git_remote, which also reports the remote host (needed for self-hosted GitHub Enterprise and GitLab) and preserves nested GitLab subgroup paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
external_href is only honored while a diff scan is being created, so a re-run over the same before/after pair left the Dashboard report with no link back to its pull request. Send on_duplicate=update alongside it, which applies the link to the existing diff scan and answers 200 with the same envelope as a create. The 409-and-resolve path is retained for runs with no pull request context and for deployments that predate on_duplicate=update. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
create_full_scan_with_report_url only fetched SBOM data when an alert-bearing output format was enabled, so --generate-license and --legal-format fossa saw an empty diff.packages and wrote an attribution file with zero packages. That is the list they enumerate, as _requires_unchanged_artifacts already documents for the comparison path. Fetch the SBOM for them too, and enrich it through the PURL endpoint the way the comparison path does. The full scan's package map is keyed by artifact id while get_license_text_via_purl keys off ecosystem/name@version, so pass a purl-keyed view over the same Package objects. Alert consolidation stays behind its own gate, so an alert-only run does not pay for the license lookup and a license-only run does not build an alert list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two ways an SCM branch build could still be treated like a pull request: Buildkite always sets BUILDKITE_PULL_REQUEST, to the string "false" on a branch build, so the documented --pr-number "$BUILDKITE_PULL_REQUEST" form delivers a truthy non-numeric value. resolve_pull_request_context read it as no PR but only wrote the normalized number back when one was found, so GithubConfig still saw "false", check_event_type returned "diff" for a push, and comment lookups went to issues/false/comments. Canonicalize config.pr_number before any adapter reads it. A branch run creating a full scan then blocked on diff.new_alerts, which a full scan cannot fill meaningfully: empty with no alert-bearing output format enabled, and every alert in the scan rather than the newly introduced ones with one. The exit code therefore depended on which output format was requested. Treat these runs the way a run with no supported manifest files is already treated and skip blocking, leaving pull request pipelines to enforce policy. Move the scan-type decision into create_scm_scan, which returns the diff and whether it came from a comparison, so the branch is exercised by tests rather than only its predicate. Document both the scan-type table and the blocking consequence in the CI/CD guide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ignore matching strips the ecosystem off a command so an ecosystem-qualified reply still matches the bare package name parsed out of a start-socket-alert marker. It stripped any leading path segment, and a scope sits in the same position, so "ignore @types/node@*" also suppressed alerts for a package named node. Only strip a leading segment that cannot be a scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Labelling every dependency overview row with bold text dropped the badge from added rows, which is the only category the overview rendered before. The badge host publishes diff-added.svg and diff-updated.svg but nothing for removed or replaced, so look the badge up per change type and fall back to the text label only where there is no image to render. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
normalized_defaults has no reader outside the branch that fills it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The entry still described the intermediate behavior where explicit diff flags opted a non-PR run into comparison mode; the detected event type has been authoritative since that was reverted. Record the blocking and license consequences alongside it, plus the ignore and overview fixes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
process_original_security_comment split the package cell on every "@", so a scoped name carrying its own "@" unpacked into three values and raised an uncaught ValueError. Same bug class this branch already fixed one function over in process_updated_security_comment, just left in its sibling. Split from the right, and pass the ecosystem through as pkg_type rather than pre-concatenating it onto the package name. That makes the two comment formats agree: both now accept an ignore command for a scoped package in either the ecosystem-qualified or the bare form, where the legacy path previously matched only the qualified one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An @SocketSecurity ignore command suppresses a security finding, but the CLI honored one from any commenter. Comment.author_association was carried on the dataclass and never read, so nothing on the path from comment to suppressed alert asked whether the author could push to the repository. A drive-by ignore-all on an open pull request silenced every finding on it. Gate the ignore bucket in check_for_socket_comments, the one place every consumer goes through. A rejected command is logged with its author and is also absent from the ignore telemetry, which should record what was acted on. GitHub returns author_association with every comment, so the check is free and definitive: OWNER, MEMBER and COLLABORATOR only. GitLab notes carry no equivalent, so project membership is read once per run, and only when an ignore command is actually present. members/all is used rather than a per-user lookup because it answers non-membership with a 200 and an absent id -- CliClient collapses every HTTP error into APIFailure without a status code, so a per-user 404, exactly the outsider case, would be indistinguishable from a token that cannot read the endpoint and would have to fail open. When membership genuinely cannot be read -- a CI_JOB_TOKEN typically cannot -- the command is honored and a warning names the author, so this does not silently break pipelines already relying on ignore commands. Documented alongside the token requirement to get enforcement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GITHUB_SERVER_URL and CI_SERVER_URL were composed into the pull request link verbatim, while the sibling repository URLs read from the same environment already went through a scheme/netloc check. The result is sent to the API as a diff scan's external_href, so route all of them through one validator. Standard runners set these themselves, so this is defense in depth rather than a live hole. An unusable value now falls back to github.com for GitHub; GitLab has no public default host, so the link is dropped and the scan keeps its number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The loop covers updated_packages as well as new_packages; the docstring still described only the latter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
36ce990 to
bd2a3c3
Compare
|
Eric Hibbs (@flowstate) thanks — all four items addressed, plus the rebase. Replies are inline on the three threads; summarizing the two that had no anchor line: Adjacent crash bug in While in there I also stopped it pre-concatenating the ecosystem onto
Two other things in this push: Version is now 2.9.0, not 2.8.0. 2.8.0 and 2.8.1 both shipped from main while this was open, so the original bump was dead. Minor rather than patch per the repo's semver standard, since this changes the behavior of existing flags rather than only fixing them: Rebased onto 642 passing, 2 skipped. |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Sweep of every comment this branch adds, against the fourth-wall skill:
- A test docstring stated the scan type "(since 2.8.0)", which was already
wrong after the renumber to 2.9.0 and would rot again on the next one. Version
stamps in comments describe a debut rather than the behavior.
- Two docstrings narrated the failure the old parser produced instead of the
invariant that makes rsplit correct. A scoped name carrying its own "@" is the
whole reason; the traceback it used to raise is not.
- The "do NOT use on_duplicate=redirect" landmine was explained twice, in full,
at both call sites. Kept at the 409 fallback, where the temptation to add it
lives; the create site now just says what update does.
- A test section header justified its own design to a reviewer ("swapping the
call back ... fails them"). Restated as what the test actually pins.
- "out of this branch" in the remote-URL regex reads as a git branch in this
repo; it means the regex case.
642 passed, ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lelia
changed the title
Each row of the legacy comment table was unpacked through four consecutive splits with no bounds checks: five cells, then the markdown link, then the ecosystem, then the version. The row comes back from the provider's API, so a cell carrying an extra "|", a package cell that is not a link, or a name with no version raised out of the comment rewrite and ended the run before it reported status. A scoped package name in Socket's own table reached the same place with nobody doing anything unusual. parse_alert_table_row returns None instead of raising for any row it cannot read, and an unreadable row keeps its alert reported -- the safe direction, since a row that cannot be parsed cannot be evaluated against the ignore commands either. Also pins change-type preservation against the real artifact conversion rather than a stubbed field. The existing test assigned diffType by hand, so it would have passed whether or not the conversion populated it; the new one runs real DiffArtifact objects through both response shapes, and fails if the field is dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 7a986c8. Configure here.
The write-access gate had no escape hatch, and its GitLab behavior when project
membership cannot be read -- honor the command with a warning -- was the one
deliberate weakness in it. Both are now a choice:
enforce (default) require write access; honor with a warning where the
provider cannot report it
strict reject in that case instead
off perform no check
enforce closes the hole wherever the provider can answer without breaking a
pipeline whose token cannot read membership, which is why it is the default.
strict closes it everywhere and will fail those pipelines. off restores the prior
behavior for anyone who needs comment-driven ignores from unverified authors.
Threaded through the adapter constructors as a keyword argument with a default, so
existing call sites keep working. With off the predicate is never handed to
check_for_socket_comments at all, so nothing is filtered and no rejection is
logged, rather than a gate that silently approves everything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Manifest paths and sources are file paths inside the scanned repository, so anyone who can open a pull request controls them: a directory named with link or tag syntax, holding a manifest, put that markup into a comment posted by a trusted integration. Alert text comes from the API. Neither is markup the CLI authored, so both are escaped where they are interpolated -- text nodes with html.escape, href and src with quotes escaped too, since an unescaped quote closes the attribute and everything after it reads as more attributes. The alert markers are the exception: they are read back verbatim when a comment is rewritten, so they cannot be escaped without breaking the ignore round trip. They instead lose only the ability to terminate the comment early. plain and raw styles are untouched. Slack, Jira and the console do not render HTML, and escaping there would show entities to a human. Round-trip tests render a comment with each hostile path and feed it back through the parser, because the renderer and the parser are two halves of one loop: an escaping choice the parser cannot read would silently stop ignores working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_get_auth_headers guesses between Bearer and PRIVATE-TOKEN from the shape of the
token, and retries once under the other scheme on a 401 so a wrong guess does not
fail the run. That retry has never executed.
Three things had to line up and none of them did:
- The retry caught requests.exceptions.HTTPError, but CliClient translates every
requests error into APIFailure before it gets there.
- CliClient discarded the HTTP status, so even a caught failure could not be
identified as a 401. is_transient_error was equally blind for the same reason.
- There are two APIFailure classes -- the CLI's own and the SDK's -- and they
were independent Exception subclasses. CliClient raises the CLI's; every
handler in socketsecurity.core imports the SDK's. None of those eight handlers
has ever caught a CliClient failure.
The CLI's APIFailure now subclasses the SDK's, so a handler written against either
catches both, and the status code travels with the exception.
The two tests covering the fallback were skipped rather than fixed, with a reason
that no longer described the failure -- the constructor they blamed is used by the
two passing tests in the same file. They now drive the exception the way CliClient
actually raises it, and fail if any of the three links above is broken again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
Behavior changes that existing pipelines will notice:
--scm githubor--scm gitlab, only pull request and merge request events create diff scans. Every other pipeline, including default-branch pushes, creates a full scan, and--enable-diffno longer opts a branch run into comparison mode.--disable-blockingwas supplied, matching how it already treats a run with no supported manifest files. Pull request and merge request pipelines still block.@SocketSecurity ignoreandignore-allare honored only from a commenter with write access to the repository. On GitHub the CLI queries and caches each commenter's effective repository permission, requiring write, maintain, or admin access instead of trusting relationship labels such asMEMBERorCOLLABORATOR. On GitLab it reads project membership. If either lookup cannot be answered, the command is honored with a warning under the defaultenforcepolicy;strictrejects indeterminate access andoffperforms no check.--pr-numberis auto-detected in GitHub Actions, GitLab CI and Azure Pipelines when omitted. An explicit value still wins, including0to disable association, and a non-numeric value such as Buildkite'sfalseis normalized to0.--scm githuband--scm gitlabimply the matching--integrationunless--integrationis supplied explicitly.Fixes:
@. The resulting error was caught and the command discarded silently, while the developer still got the acknowledgement reaction.ignore @types/node@*also suppressed alerts for a package namednode.--generate-licenseand--legal-format fossaproduced an empty attribution file on any path without a baseline, because the package list was only fetched for alert-bearing output formats.No issues found. They now state that findings were not fetched for console output and link to the Socket report.GITHUB_SERVER_URLandCI_SERVER_URLwere composed into a diff scan'sexternal_hrefwithout the scheme and host validation applied to the sibling repository URLs read from the same environment.APIFailurewas an independent type from the SDK exception of the same name that every handler catches. A misclassified token failed the run instead of falling back.Added:
firstPatchedVersionIdentifier.Implementation notes
Pull request context is resolved from explicit configuration first, then from standard provider CI variables, and the normalized number is stored before any comment adapter reads it.
Repository slugs and hosts are recovered from
BUILDKITE_REPOthrough one shared parser, so nested GitLab subgroups and self-hosted installations produce correct links without extra configuration.The diff scan link is sent with
on_duplicate=update. Deployments that answer 409 regardless keep the existing conflict-resolution fallback.Ignore authorization is gated where comments are bucketed rather than at each consumer, so a rejected command reaches neither the alert filter nor the ignore telemetry, and no acknowledgement reaction is added to a comment that was not honored. GitHub effective permissions are cached per commenter; GitLab membership is cached per run. Generated ignore commands are ecosystem-qualified; parsing stays compatible with older bare-name replies and splits a scoped name at its final
@.Full scans carry explicit output metadata so console summaries can distinguish “no fetched findings” from a verified empty result without paying for an otherwise unnecessary SBOM fetch.
Testing
686 passed, 0 skippedacrosstests/unitandtests/coreruff checkclean across the repositoryuv lock --lockedsucceedsuv buildUpgrade notes
Minor rather than patch: this changes the behavior of existing flags rather than only fixing them.
A pipeline that relied on a default-branch push failing the build on new alerts will now pass; use a pull request pipeline, or
--enable-diffwithout--scm, to keep a blocking comparison. A pipeline that relied on ignore commands from a commenter without write access will start reporting those alerts. Supply a GitHub token with repository metadata access or a GitLab token that can read project membership to verify permissions;--ignore-authorization strictrejects commands when access cannot be determined,offrestores the previous behavior outright, and--disable-ignoreturns comment-driven ignores off entirely.Fixes: CE-426, CE-459
Refs: CE-94, CE-215, CE-337, CE-376, CE-424, CE-441
Note
Medium Risk
Changes default CI blocking for SCM branch pipelines and tightens who can suppress alerts via PR comments; behavior is documented but may surprise existing workflows relying on branch-build failures or outsider ignore commands.
Overview
Release 2.9.0 reshapes how GitHub/GitLab SCM runs choose scan type, tie results to CI change context, and who can suppress findings from PR/MR comments.
With
--scm githubor--scm gitlab, only PR/MR events run diff scans; default-branch and other branch pipelines now create full scans and behave like--disable-blockingwas set (PR/MR pipelines still block).--enable-diffno longer forces comparison on those branch runs. Full scans also fetch SBOM data when--generate-license/ FOSSA attribution is enabled so branch pipelines do not emit empty license files. Console-only full scans link to the Socket report instead of treating an unfetched alert list as a clean result.Dashboard PR association is expanded:
--pr-numberauto-detects in GitHub Actions, GitLab CI, and Azure Pipelines; non-numeric values (e.g. Buildkitefalse) normalize to no PR.--scm github|gitlabimplies matching--integrationwhen integration is omitted. Diff scans send anexternal_hrefPR/MR URL (withon_duplicate=updateon re-runs); Buildkite uses sharedBUILDKITE_REPOparsing for slug/host.@SocketSecurity ignoreis accepted only from commenters with write access (GitHub effective repository permission; GitLab project membership, with the configured fallback policy when an access lookup is unreadable). Ignore matching and generated instructions use ecosystem-qualified names, fix scoped-package parsing, and harden legacy comment-table row parsing.Human-readable output adds patched version from
firstPatchedVersionIdentifier; dependency overviews distinguish added/updated/removed/replaced; GitLab comment copy is provider-neutral; namespaced packages retain their namespace during license enrichment.Reviewed by Cursor Bugbot for commit 7a986c8. Configure here.